-
Notifications
You must be signed in to change notification settings - Fork 249
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
zcash_client_sqlite: Fix migration DAG edges #1506
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK modulo clippy failures.
init_wallet_db_internal( | ||
&mut db_data, | ||
Some(Secret::new(seed_bytes.clone())), | ||
&DEPENDENCIES, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good way to approach migration tests; it ensures that only the nodes in the graph leading up to this node are evaluated, so anything that's missing should get caught. It does require inspecting what data is required in order to test all the possible branches within the migration though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK e4b3fb2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just registering a block on merging because I'm doing an exhaustive analysis of whether tx_retrieval_queue
depends semantically on any of the v0.11.1 leaf migrations (which the test will not necessarily catch). It shouldn't take too long.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1506 +/- ##
=======================================
Coverage 61.15% 61.16%
=======================================
Files 141 141
Lines 16653 16649 -4
=======================================
- Hits 10184 10183 -1
+ Misses 6469 6466 -3 ☔ View full report in Codecov by Sentry. |
01995dc
to
498dfe5
Compare
force-pushed to make |
a224a65
to
3a52fb5
Compare
]; | ||
|
||
/// Leaf migrations in the 0.4.0 release. | ||
const V_0_4_0: &[Uuid] = &[add_transaction_views::MIGRATION_ID]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I verified that {v0.4.1, v0.4.2, v0.5.0} did not add migrations relative to v0.4.0, and at v0.4.2 the ASCII art was correct. In all of these versions, add_transaction_views
was indeed the only leaf.
3a52fb5
to
3dba2fa
Compare
const V_0_4_0: &[Uuid] = &[add_transaction_views::MIGRATION_ID]; | ||
|
||
/// Leaf migrations in the 0.6.0 release. | ||
const V_0_6_0: &[Uuid] = &[v_transactions_net::MIGRATION_ID]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I verified that as of v0.6.0 the ASCII art was correct, and v_transactions_net
was the only leaf.
|
||
/// Leaf migrations in the 0.6.0 release. | ||
const V_0_6_0: &[Uuid] = &[v_transactions_net::MIGRATION_ID]; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, at v0.7.0 a received_notes_nullable_nf
migration was added that was not documented in the ASCII art.
/// Leaf migrations in the 0.7.0 release. | |
const V_0_7_0: &[Uuid] = &[received_notes_nullable_nf::MIGRATION_ID]; | |
Non-blocking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK
No description provided.